-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docs: host-gateway-ip daemon option IPv4+IPv6 #5607
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5607 +/- ##
=======================================
Coverage 59.53% 59.53%
=======================================
Files 346 346
Lines 29357 29357
=======================================
Hits 17477 17477
Misses 10910 10910
Partials 970 970 |
f92dc3f
to
1864129
Compare
docs/reference/dockerd.md
Outdated
@@ -62,8 +62,7 @@ Options: | |||
-G, --group string Group for the unix socket (default "docker") | |||
--help Print usage | |||
-H, --host list Daemon socket(s) to connect to | |||
--host-gateway-ip ip IP address that the special 'host-gateway' string in --add-host resolves to. | |||
Defaults to the IP address of the default bridge | |||
--host-gateway-ip list IP addresses that the special 'host-gateway' string in --add-host resolves to. Defaults to the IP addresses of the default bridge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Can you manually wrap / format this one? This page is still manually maintained and this block rendered as a code-block, so wrapping reduces horizontal scrolling; see https://docs.docker.com/reference/cli/dockerd/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do ... I did that at first, then thought maybe it was supposed to exactly match the --help
output. Doh!
host-gateway-ip
isn't a new option, but it's not in the manpage in this repo or moby ... I'll add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we really need to;
- update our packaging to use the man-page from moby/moby instead (and remove the man page from this repo)
- look at moving the dockerd reference docs to moby/moby, and use the "docs-generator" to generate these flag-descriptions and the "YAML" docs so that we can use the same formatting as the other reference docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed the dockerd.md
formatting, and added --host-gateway
to the manpage (also put --help
in the right place).
I'll make the same changes to moby's copy of the manpage.
The host-gateway-ip daemon option now accepts two addresses, one IPv4 and one IPv6. Signed-off-by: Rob Murray <[email protected]>
Signed-off-by: Rob Murray <[email protected]>
Signed-off-by: Rob Murray <[email protected]>
1864129
to
c629eca
Compare
@@ -63,8 +63,8 @@ Options: | |||
-G, --group string Group for the unix socket (default "docker") | |||
--help Print usage | |||
-H, --host list Daemon socket(s) to connect to | |||
--host-gateway-ip ip IP address that the special 'host-gateway' string in --add-host resolves to. | |||
Defaults to the IP address of the default bridge | |||
--host-gateway-ip list IP addresses that the special 'host-gateway' string in --add-host resolves to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate Cobra for this; we should probably look if we can make it show a custom type. "list" is ... not very informative. The ugly bit is that Cobra doesn't allow you to set it per flag, but it's really tied to the type you're using 😞 (perhaps we can come with some trickery to initialise it for a specific flag 🤔)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We almost have the trickery, it's not just used / there's a bit of plumbing missing - it's not configurable in a ListOpts
so those options report list
.
For ListOpts
/ NamedListOpts
it comes from...
https://github.com/moby/moby/blob/af0b973595323cbd43132624ee2204af9a503588/opts/opts.go#L109-L112
For the new NamedIPListOpts
, it's here ... https://github.com/moby/moby/blob/af0b973595323cbd43132624ee2204af9a503588/internal/opts/named_iplist_opts.go#L41-L43
So, we could come up with a better scheme if we wanted to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's not really a new problem, but something we should look at across the board. Things like string
are pretty useless to the user, and ideally these would show <some type that can be documented>
. Similar to, e.g. man wget
would show more specific types (ISTR I found better examples, but couldn't remember where);
-e command
--execute command
Execute command as if it were a part of .wgetrc. A command thus invoked will be executed after the commands in .wgetrc, thus
taking precedence over them. If you need to specify more than one wgetrc command, use multiple instances of -e.
Logging and Input File Options
-o logfile
--output-file=logfile
Log all messages to logfile. The messages are normally reported to standard error.
-a logfile
--append-output=logfile
Append to logfile. This is the same as -o, only it appends to logfile instead of overwriting the old log file. If logfile
does not exist, a new file is created.
If such types are somewhat standardised in our codebase, that would also allow them to be documented, e.g.
--platform PLATFORM
--gpus gpu-request
--health-interval duration
Then we could even provide links to those formats in our docs (i.e., here's where the format is described for platform
or ip
or duration
).
Ideally, we'd also come with a format that can indicate "can be specified multiple times" ([]platform
? other options?)
There's some fun bug/issue still to fix for that though, because Cobra currently requires "multiple times" to be suffixed by List
or Array
to make auto-completion work properly 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
- What I did
Depends on:
The host-gateway-ip daemon option now accepts two addresses, one IPv4 and one IPv6.
- How I did it
- How to verify it
- Description for the changelog